DATAJDBC-256 - Run integration tests with Oracle #103
Conversation
Signed-off-by: Michael Bahr <michael.bahr@live.de>
Signed-off-by: Michael Bahr <michael.bahr@live.de>
There was a problem hiding this comment.
This looks pretty good.
I left some detailed wishes.
I'm not sure why you have almost all integration tests disabled since at least one, possibly many should be easy to make work.
See 4208511
Maybe this is due to a misunderstanding of what I said earlier.
It is ok to disable tests if you can't make it succeed.
In those cases, I need a comment explaining what the problem is.
Ideally just a link to a Jira issue which contains the explanation.
| @@ -44,12 +38,22 @@ | |||
| import org.springframework.lang.Nullable; | |||
| import org.springframework.util.Assert; | |||
|
|
|||
| import java.sql.Connection; | |||
There was a problem hiding this comment.
Please make sure to configure your code formatter: https://github.com/spring-projects/spring-data-build/tree/master/etc/ide
...jdbc/src/test/java/org/springframework/data/jdbc/core/AggregateTemplateIntegrationTests.java
Show resolved
Hide resolved
| @@ -339,6 +346,31 @@ public long count(Class<?> domainType) { | |||
| } | |||
| } | |||
|
|
|||
| Optional<String> getIdColumnNameIfOracle(@Nullable final RelationalPersistentProperty idProperty) { | |||
There was a problem hiding this comment.
This logic should get moved out of the DefaultDataAccessStrategy.
To be more specific:
-
create an interface
DatabaseDialectwith a methodboolean registerGeneratedColumns. -
provide two implementations: One for Oracle and one default one.
-
Inject it into the Data
-
add a
DatabaseDialectbean toJdbcConfiguration -
Inject the bean into
DefaultDataAccessStrategy -
For tests provide the Oracle variant in the
OracleDataSourceConfiguration. -
We do not need to provide detection logic, that is probably better done by Spring Boot.
| parameters.forEach(parameterSource::addValue); | ||
|
|
||
| operations.update( // | ||
| sql(domainType).getInsert(parameters.keySet()), // | ||
| parameterSource, // | ||
| holder // | ||
| holder, // | ||
| idColumnName.isPresent() ? new String[]{idColumnName.get()} : (String[])null // |
There was a problem hiding this comment.
We probably need this to also depend on if the id is already set.
| @Override | ||
| protected void customizePopulator(ResourceDatabasePopulator populator) { | ||
| populator.setIgnoreFailedDrops(true); | ||
| populator.setSeparator("§"); |
There was a problem hiding this comment.
| populator.setSeparator("§"); | |
| populator.setSeparator("/"); |
The paragraph symbol is hard to type (I don't think I have it on my keyboard) and the / is pretty much Oracles standard for this purpose.
|
Thanks for the feedback! I'll look into it over the holidays. |
…ests Signed-off-by: Michael Bahr <michael.bahr@live.de>
| @@ -0,0 +1 @@ | |||
| oracle.container.image=wnameless/oracle-xe-11g@sha256:825ba799432809fc7200bb1d7ef954973a8991d7702a860c87177fe05301f7da | |||
There was a problem hiding this comment.
We must not use this docker image since it is not on supported by Oracle and quite possibly illegal to use.
Please only use official Oracle images: https://github.com/oracle/docker-images
There was a problem hiding this comment.
That might make things a lot easier (e.g. ID columns). Somehow previously I only found images that one would have to build locally. I assume requiring a docker login for pulling the Oracle images before running the Oracle tests is ok? (note that the downloaded docker image is 5GB+)
There was a problem hiding this comment.
Yes, a login to Docker is ok.
|
Hey, guys, it would be very very nice to have this PR merged |
We're not ready for merging it yet. I had a couple rather busy weeks/months, but should find more time to finish this soon :) |
|
Also, note this is just about being able to execute the tests and fixing the obvious and easy issues. We'll probably need another issue for making it actually work with Oracle. |
|
I will stop working on this. Even with Oracle 12 I don't see this MR to be properly completed within a reasonable time. Thank you for supporting me in trying. |
|
That is unfortunate but perfectly understandable. Thanks for your work I'll take it from here then. |
…ion. RowsFetchSpec.awaitOne() and RowsFetchSpec.awaitFirst() now throw EmptyResultDataAccessException instead of NoSuchElementException to consistently use Spring DAO exceptions.
ASon joins as Oracle does not understand thisAggregateTemplateIntegrationTestsandIfProfileValuewhere I haven't added Oracle setup scripts yet. I used§as a custom delimiter as it was the first I found, can be changed. The custom delimiter is however necessary to allow the PL/SQL style setup of triggers for id generation.mvn test -Pall-dbs.